feat(mysql): #277 STRICT_ALL_TABLES required sql mode support #327
Conversation
ghost
assigned
lib/db/mysql.js
Outdated
|
|
||
| connection.query('SET NAMES utf8mb4 COLLATE utf8mb4_bin;', (err) => { | ||
| var mode = REQUIRED_SQL_MODES.join(',') | ||
| connection.query('SET SESSION sql_mode = \'' + mode + '\'', function(err) { |
There was a problem hiding this comment.
can we use (err) => { here?
lib/db/mysql.js
Outdated
|
|
||
| connection.query('SET NAMES utf8mb4 COLLATE utf8mb4_bin;', (err) => { | ||
| var mode = REQUIRED_SQL_MODES.join(',') | ||
| connection.query('SET SESSION sql_mode = \'' + mode + '\'', function(err) { |
There was a problem hiding this comment.
can this use ` ` for the query here? might make it cleaner https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
lib/db/mysql.js
Outdated
| connection.query('SET SESSION sql_mode = \'' + mode + '\'', function(err) { | ||
| if (err) { | ||
| return reject(err) | ||
| return reject(err); |
There was a problem hiding this comment.
no semicolons in this repo :(
|
@rfk made the changes, will squash commits after review :) |
lib/db/mysql.js
Outdated
|
|
||
| var mode = REQUIRED_SQL_MODES.join(',') | ||
| connection.query('SET SESSION sql_mode = \'' + mode + '\'', function(err) { | ||
| connection.query(`SET SESSION sql_mode = ${mode};`, (err) => { |
There was a problem hiding this comment.
you might need the ' ' around ${mode} per example in https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html
SET SESSION sql_mode = 'modes';
There was a problem hiding this comment.
Yeah i changed it after this, need to squash commits :(
| .then( | ||
| assert.fail, | ||
| err => { | ||
| assert.equal(err.message, `You cannot use any sql mode other than ${mode}`) |
There was a problem hiding this comment.
👍 great to have this test to make sure!
rfk
left a comment
rfk
left a comment
There was a problem hiding this comment.
Thanks @deeptibaghel, this looks good to me!
We'll need to make an explicit note of this in the deploy doc, and watch out for errors after rolling it out to production. I give it a low-single-digit-percentage chance of uncovering some weird bug when run in production (which is the idea, we just have to be prepared for it).
|
Thanks @rfk , have squashed the commits :) |
|
Thank you @deeptibaghel !! |
3 participants
Fixes #277